Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(controller): add support for mute timing #1817

Merged
merged 1 commit into from
Jan 27, 2025
Merged

feat(controller): add support for mute timing #1817

merged 1 commit into from
Jan 27, 2025

Conversation

chenlujjj
Copy link
Contributor

To resolve: #1723

@chenlujjj chenlujjj marked this pull request as draft January 11, 2025 13:31
@chenlujjj chenlujjj force-pushed the feat/mute-timing branch 7 times, most recently from 2420950 to f0b9e9b Compare January 11, 2025 15:07
@chenlujjj chenlujjj marked this pull request as ready for review January 11, 2025 15:07
@chenlujjj
Copy link
Contributor Author

tested in local:

---
apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaMuteTiming
metadata:
  name: mutetiming-sample
spec:
  instanceSelector:
    matchLabels:
      dashboards: "grafana"
  name: mutetiming-sample
  editable: false
  time_intervals:
    - times:
        - start_time: "00:00"
          end_time: "06:00"
      weekdays: ["saturday", "sunday"]
      days_of_month: ["1", "10"]
      location: "Asia/Shanghai"

result:
image

@theSuess
Copy link
Member

Thanks for the PR! I'll take a look at this soon. It's too big of a change for the next release, but we'll get it out soon now that the holiday season is over

@theSuess theSuess added this to the v5.17.0 milestone Jan 13, 2025
@theSuess theSuess added the feature this PR introduces a new feature label Jan 15, 2025
Baarsgaard
Baarsgaard suggested changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@Baarsgaard Baarsgaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These along with a rebase onto master should be mostly it.

api/v1beta1/grafanamutetiming_types.go Show resolved Hide resolved
api/v1beta1/grafanamutetiming_types.go Outdated Show resolved Hide resolved
controllers/grafanamutetiming_controller.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the documentation Issues relating to documentation, missing, non-clear etc. label Jan 22, 2025
theSuess
theSuess requested changes Jan 23, 2025
View reviewed changes
Copy link
Member

@theSuess theSuess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great - sorry for the late review!

Once the minor comments I left are addressed, I'm happy to merge this. Will definitely make it into the next release

controllers/grafanamutetiming_controller.go Outdated Show resolved Hide resolved
controllers/grafanamutetiming_controller.go Outdated Show resolved Hide resolved
api/v1beta1/grafanamutetiming_types.go Outdated Show resolved Hide resolved
api/v1beta1/grafanamutetiming_types.go Outdated Show resolved Hide resolved
@chenlujjj
Copy link
Contributor Author

chenlujjj commented Jan 26, 2025 •
edited
Loading

@theSuess

I think we should go with a plain bool (& default to true)

When I comment Editable bool with // +kubebuilder:default=true, it seems that even the field is set as false explicitly, it will change to true after applied to k8s. That's why the test case fails now. Did I miss some corner case for kubebuilder ?

@theSuess
Copy link
Member

I'll take a look at this, thanks for pointing it out!

@chenlujjj
Copy link
Contributor Author

I think it cannot tell whether the Editable field is not set or set as false because zero value of bool type is false.
So we have two choices:

  • bool type and default to false
  • *bool type and default to nil

@theSuess
Copy link
Member

I've found a fix for this in the comments of this issue and pushed it to the branch. If the tests pass, I'll merge this pr!

@theSuess theSuess added this pull request to the merge queue Jan 27, 2025
@theSuess
Copy link
Member

Thanks again for all the work on this @chenlujjj! The feature is now queued for merge. I'll put out a release once the changes around notification policies are merged as well

Merged via the queue into grafana:master with commit e97a39e Jan 27, 2025
15 checks passed
@chenlujjj
Copy link
Contributor Author

@theSuess Thank you for get the issue fixed and happy to see it merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Baarsgaard Baarsgaard Baarsgaard requested changes

@theSuess theSuess theSuess approved these changes

@NissesSenap NissesSenap Awaiting requested review from NissesSenap NissesSenap is a code owner

@weisdd weisdd Awaiting requested review from weisdd weisdd is a code owner

@ishanjainn ishanjainn Awaiting requested review from ishanjainn ishanjainn is a code owner

@hubeadmin hubeadmin Awaiting requested review from hubeadmin hubeadmin is a code owner

@pb82 pb82 Awaiting requested review from pb82 pb82 is a code owner

Assignees
No one assigned
Labels
documentation Issues relating to documentation, missing, non-clear etc. feature this PR introduces a new feature
Projects
None yet
Milestone
v5.17.0
Development

Successfully merging this pull request may close these issues.

Feature: Provision mute time intervals trough the operator
3 participants
@chenlujjj @theSuess @Baarsgaard